Skip to content

Conversation

@jhixson74
Copy link
Member

This brings Azure private DNS into the 4.2 branch

https://issues.redhat.com/browse/CORS-1316

…ndle private_dns zone

Using the upstream azurerm provider is not possible for now because of following reasons:

1) There is not srv record resource for private dns zone

2) The version of provider that has the private dns zone resources `1.34.0` has a lot of bugs like
    * hashicorp/terraform-provider-azurerm#4452
    * hashicorp/terraform-provider-azurerm#4453
    * hashicorp/terraform-provider-azurerm#4501
    Some of these bugs are fixed, and some are in flight.

    Another reason moving to `1.36.0` which might have all the fixes we need is the provider has moved to using
    `standalone terraform plugin SDK v1.1.1` [1]. Because we vendor both terraform and providers, this causes errors like
    `panic: gob: registering duplicate types for "github.com/zclconf/go-cty/cty.primitiveType": cty.primitiveType != cty.primitiveType`

   Therefore, we would have to move towards a single vendor for terraform and plugins for correct inter-operation, which is tricker due to conflicts elsewhere

A simple 4 resource plugin that re-uses the already vendored azurerm provider as library and carries over the required resources seems like an easy fix for now.

[1]: hashicorp/terraform-provider-azurerm#4474

(cherry picked from commit af00810)
Using the Private DNS Zone also allows us to remove our previous hack [1]

[1]: openshift@8ac9ab4

(cherry picked from commit c1e24af)

Conflicts:
	data/data/azure/master/variables.tf
the ingress-operator can handle the Private DNS Zone since [1]

[1]: openshift/cluster-ingress-operator#300

(cherry picked from commit 7d3119f)
… deleting public records

Updates the destroy to look for both DNS Zones type Private and Private DNS Zones to find the private records and the corresponding DNS Zone type Public.

Since the zone name for both types of private dns zone is the same, we can try both to calculate the private records and then re-use the same codepath to delete the public records.

(cherry picked from commit 65111a0)
@openshift-ci-robot
Copy link
Contributor

@jhixson74: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

WIP: backport Azure private DNS to 4.2 branch

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 16, 2019
@jhixson74 jhixson74 changed the title WIP: backport Azure private DNS to 4.2 branch Backport Azure private DNS to 4.2 branch Dec 16, 2019
@openshift-ci-robot
Copy link
Contributor

@jhixson74: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

Backport Azure private DNS to 4.2 branch

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 16, 2019
@jhixson74
Copy link
Member Author

/test e2e-azure

@jhixson74 jhixson74 changed the title Backport Azure private DNS to 4.2 branch Bug 1788707: Backport Azure private DNS to 4.2 branch Jan 7, 2020
@openshift-ci-robot
Copy link
Contributor

@jhixson74: This pull request references Bugzilla bug 1788707, which is invalid:

  • expected the bug to target the "4.2.z" release, but it targets "---" instead
  • expected Bugzilla bug 1788707 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1788707: Backport Azure private DNS to 4.2 branch

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jan 7, 2020
@openshift-ci-robot
Copy link
Contributor

@jhixson74: This pull request references Bugzilla bug 1788707, which is invalid:

  • expected the bug to target the "4.2.z" release, but it targets "---" instead
  • expected Bugzilla bug 1788707 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jhixson74
Copy link
Member Author

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@jhixson74: This pull request references Bugzilla bug 1788707, which is invalid:

  • expected the bug to target the "4.2.z" release, but it targets "---" instead
  • expected Bugzilla bug 1788707 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jhixson74
Copy link
Member Author

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@jhixson74: This pull request references Bugzilla bug 1788707, which is invalid:

  • expected Bugzilla bug 1788707 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jhixson74
Copy link
Member Author

/bugzilla refresh

@openshift-ci-robot
Copy link
Contributor

@jhixson74: This pull request references Bugzilla bug 1788707, which is invalid:

  • expected the bug to be in one of the following states: NEW, ASSIGNED, ON_DEV, POST, POST, but it is VERIFIED instead
  • expected Bugzilla bug 1788707 to depend on a bug in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but no dependents were found

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@mtnbikenc
Copy link
Member

/bugzilla refresh

@openshift-ci-robot openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jan 8, 2020
@openshift-ci-robot
Copy link
Contributor

@mtnbikenc: This pull request references Bugzilla bug 1788707, which is valid. The bug has been moved to the POST state.

Details

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jhixson74
Copy link
Member Author

/test e2e-azure

@wking
Copy link
Member

wking commented Jan 9, 2020

There's no 4.3 analog to this installer PR, and the 4.3 rhbz#1772804 is VERIFIED with no installer changes. Is that intentional? Can you explain why 4.3+ don't need this installer change?

@wking
Copy link
Member

wking commented Jan 9, 2020

Azure job died with:

fail [github.com/openshift/origin/test/extended/prometheus/prometheus_builds.go:135]: Expected
    <map[string]error | len:1>: {
        "ALERTS{alertname!=\"Watchdog\",alertstate=\"firing\"} >= 1": {
            s: "promQL query: ALERTS{alertname!=\"Watchdog\",alertstate=\"firing\"} >= 1 had reported incorrect results: ALERTS{alertname=\"ClusterMonitoringOperatorDown\", alertstate=\"firing\", severity=\"critical\"} => 1 @[1578596289.224]\nALERTS{alertname=\"MachineAPIOperatorDown\", alertstate=\"firing\", severity=\"critical\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"api\", namespace=\"openshift-apiserver\", service=\"api\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"catalog-operator-metrics\", namespace=\"openshift-operator-lifecycle-manager\", service=\"catalog-operator-metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"cluster-monitoring-operator\", namespace=\"openshift-monitoring\", service=\"cluster-monitoring-operator\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"controller-manager\", namespace=\"openshift-controller-manager\", service=\"controller-manager\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"dns-default\", namespace=\"openshift-dns\", service=\"dns-default\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"machine-api-operator\", namespace=\"openshift-machine-api\", service=\"machine-api-operator\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-apiserver-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-authentication-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-controller-manager-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-kube-apiserver-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-kube-controller-manager-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-kube-scheduler-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"olm-operator-metrics\", namespace=\"openshift-operator-lifecycle-manager\", service=\"olm-operator-metrics\", severity=\"warning\"} => 1 @[1578596289.224]",
        },
    }
to be empty

That's a MachineAPIOperatorDown and a bunch of TargetDown. I didn't dig deeper to see if they were related to this PR or not.

@jhixson74
Copy link
Member Author

There's no 4.3 analog to this installer PR, and the 4.3 rhbz#1772804 is VERIFIED with no installer changes. Is that intentional? Can you explain why 4.3+ don't need this installer change?

This code is a feature in 4.3. The same commits in this PR are already in 4.3. This PR is for back porting the changes to 4.2.

@jhixson74
Copy link
Member Author

Azure job died with:

fail [github.com/openshift/origin/test/extended/prometheus/prometheus_builds.go:135]: Expected
    <map[string]error | len:1>: {
        "ALERTS{alertname!=\"Watchdog\",alertstate=\"firing\"} >= 1": {
            s: "promQL query: ALERTS{alertname!=\"Watchdog\",alertstate=\"firing\"} >= 1 had reported incorrect results: ALERTS{alertname=\"ClusterMonitoringOperatorDown\", alertstate=\"firing\", severity=\"critical\"} => 1 @[1578596289.224]\nALERTS{alertname=\"MachineAPIOperatorDown\", alertstate=\"firing\", severity=\"critical\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"api\", namespace=\"openshift-apiserver\", service=\"api\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"catalog-operator-metrics\", namespace=\"openshift-operator-lifecycle-manager\", service=\"catalog-operator-metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"cluster-monitoring-operator\", namespace=\"openshift-monitoring\", service=\"cluster-monitoring-operator\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"controller-manager\", namespace=\"openshift-controller-manager\", service=\"controller-manager\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"dns-default\", namespace=\"openshift-dns\", service=\"dns-default\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"machine-api-operator\", namespace=\"openshift-machine-api\", service=\"machine-api-operator\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-apiserver-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-authentication-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-controller-manager-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-kube-apiserver-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-kube-controller-manager-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"metrics\", namespace=\"openshift-kube-scheduler-operator\", service=\"metrics\", severity=\"warning\"} => 1 @[1578596289.224]\nALERTS{alertname=\"TargetDown\", alertstate=\"firing\", job=\"olm-operator-metrics\", namespace=\"openshift-operator-lifecycle-manager\", service=\"olm-operator-metrics\", severity=\"warning\"} => 1 @[1578596289.224]",
        },
    }
to be empty

That's a MachineAPIOperatorDown and a bunch of TargetDown. I didn't dig deeper to see if they were related to this PR or not.

The only problem I am aware of as to why the Azure test will fail is the ingress operator not having support for the new Azure private DNS in 4.2. The PR for that (openshift/cluster-ingress-operator#344) merged today, so the test should now work. I am not clear on if this change is immediate or if there is a delay before it is available for CI.

@jhixson74
Copy link
Member Author

/test e2e-azure

@wking
Copy link
Member

wking commented Jan 10, 2020

I am not clear on if this change is immediate or if there is a delay before it is available for CI.

PR merging should be all that is needed before changes appear in subsequent CI jobs. But if you want to check for a particular job, can drill in like:

@wking
Copy link
Member

wking commented Jan 10, 2020

This code is a feature in 4.3.

So looks like this mostly backports #2470, but with 36e431a and 0ec99cc unique to the backport? 0ec99cc overlaps with, but is not the same as 48da378 (#2429). Oh, I see you have some commit references in your cherry-pick commit references. Looking...

@wking
Copy link
Member

wking commented Jan 10, 2020

@wking
Copy link
Member

wking commented Jan 10, 2020

@jhixson74
Copy link
Member Author

* [7d3119f](https://github.com/openshift/installer/commit/7d3119f8ba3ecefba5b1ec7168de526fdab81b6d) -> [eb1884d](https://github.com/openshift/installer/commit/eb1884da336e9b5d276f49378a1f9beaa9b5801f) is clean

* [e80fde1](https://github.com/openshift/installer/commit/e80fde1336b2c5feb400428b74364fc455b78c57) -> [36e431a](https://github.com/openshift/installer/commit/36e431a54bf18a472a1ea912d2ccac21820df22e) is clean

* [65111a0](https://github.com/openshift/installer/commit/65111a04a42cf864900086acdbb46ee86a8f9724) -> [5e8e8a5](https://github.com/openshift/installer/commit/5e8e8a5816ee6c50d9850cdfd73d10e05c800d8b) is clean

* I don't understand [0ec99cc](https://github.com/openshift/installer/commit/0ec99ccc4f6d1b68670fc8ac4b008a87a7822893) (except for the `private_dns_zone_id` recovery mentioned above).

master_subnet_cidr was nuked in 4.3 except in the vnet module, where it has become a local. The cherry pick relies on this, so that commit removed the older 4.2 references.

Copy link
Contributor

@jstuever jstuever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020
@jstuever jstuever removed their assignment Jan 13, 2020
@wking
Copy link
Member

wking commented Jan 13, 2020

/approve

@wking
Copy link
Member

wking commented Jan 14, 2020

/approve

Trying again.

@jhixson74
Copy link
Member Author

/assign @jhixson74

@wking
Copy link
Member

wking commented Jan 14, 2020

/approve

Eventually Prow will hear about this ;).

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jstuever, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2020
@crawford crawford added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Jan 21, 2020
@openshift-merge-robot openshift-merge-robot merged commit 23438fc into openshift:release-4.2 Jan 21, 2020
@openshift-ci-robot
Copy link
Contributor

@jhixson74: All pull requests linked via external trackers have merged. Bugzilla bug 1788707 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1788707: Backport Azure private DNS to 4.2 branch

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants